extstore s3 driver#2129
Conversation
6f23506 to
2f945b0
Compare
1b6c2b9 to
447552b
Compare
fa18b39 to
a9e4554
Compare
| { | ||
| "name": "@temporalio/external-storage-s3-aws-sdk", | ||
| "version": "1.18.1", | ||
| "private": true, |
There was a problem hiding this comment.
prevents npm publish from exposing these until extstore is released. we'll want to flip this later.
| async function runAllWithAbortOnError<T>( | ||
| external: AbortSignal | undefined, | ||
| makeTasks: (signal: AbortSignal) => Promise<T>[] | ||
| ): Promise<T[]> { | ||
| const controller = new AbortController(); | ||
| const signal = external ? AbortSignal.any([external, controller.signal]) : controller.signal; | ||
| const tasks = makeTasks(signal); | ||
| try { | ||
| return await Promise.all(tasks); | ||
| } catch (err) { | ||
| controller.abort(); | ||
| await Promise.allSettled(tasks); | ||
| throw err; | ||
| } | ||
| } |
There was a problem hiding this comment.
n.b.
- S3 driver's
runAllWithAbortOnErroris concurrent and cancels in-flight sibling requests on the first failure (AWS SDK allows for cancelling mid-request). - GCS driver's
allSettledOrThrowis concurrent but doesn't attempt mid-flight cancellation.@google-cloud/storagecan't cancel an in-flight request. Abort in GCS is only check up-front.
jmaeagle99
left a comment
There was a problem hiding this comment.
LGTM with a few minor comments. Please seek an TS maintainer for review as well.
|
|
||
| describe(): Record<string, string> { | ||
| const region = this.client.config?.region; | ||
| return typeof region === 'string' && region ? { clientRegion: region } : {}; |
There was a problem hiding this comment.
region might also be a Provider<string | undefined> as documented at https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/Package/-aws-sdk-client-s3/Interface/ClientInputEndpointParameters/
Do we want to invoke the provider, check if the promise is completed (assuming the promise is cached), and read the value? If this is even possible.
There was a problem hiding this comment.
My understanding is that region will be set to the string when the promise resolves which is essentially what you are suggesting. We can't check promise status.
| * @experimental | ||
| */ | ||
| export class AwsSdkS3StorageDriverClient implements S3StorageDriverClient { | ||
| constructor(private readonly client: AwsS3Client) {} |
There was a problem hiding this comment.
I think the way we are using this client, it does not support multipart upload. We might either need to depend on another package or write it ourselves using this client. There is varying recommendations of at what threshold multipart should be used, but it looks like AWS says to definitely use it at 100 MB or larger, which is above our max payload size. So maybe something to defer for now, what for feedback and/or measure with experimentation.
There was a problem hiding this comment.
Good callout. I've documented this. Agree on deferring for now.
|
|
||
| > ⚠️ **This package is experimental and may be subject to change.** ⚠️ | ||
|
|
||
| `@temporalio/external-storage-s3` stores and retrieves Temporal payloads in Amazon S3 via the [External Storage](../../README.md) feature. |
There was a problem hiding this comment.
Why does External Storage link to the README? did you mean docs: https://docs.temporal.io/external-storage
There was a problem hiding this comment.
Yea I suppose that is better! I originally had this linking to the in-repo docs.
| await Promise.allSettled(tasks); | ||
| throw err; | ||
| } | ||
| } |
There was a problem hiding this comment.
Remind me - what do we do when an operation fails, is it a task failure and then we retry?
Just thinking of the scenario when store fails partway through. Subsequent retries should attempt to store only the payloads where objectExists == false ?
What about if retries are exhausted and we've partly completed the store operation? I guess there isn't much we can do
There was a problem hiding this comment.
Yes. We fail the task and it must be retried at the task-level. If we are trying store many payloads and one fails, the whole thing fails.
There was a problem hiding this comment.
Right now, none of the drivers will retry. I think we should have some sensible retry policy to address transient failures. Will chat with the team about this.
…ould still occur and waste time/bandwidth.
eed8180 to
e5ae665
Compare
| async store(context: StorageDriverStoreContext, payloads: Payload[]): Promise<StorageDriverClaim[]> { | ||
| const contextSegments = buildContextSegments(context.target); | ||
| return runAllWithAbortOnError(context.abortSignal, (signal) => { | ||
| const uploads = new Map<string, Promise<void>>(); |
There was a problem hiding this comment.
That's an interesting optimization! Should we do the same for downloads too?
What was changed?
@temporalio/external-storage-s3: S3StorageDriver + S3StorageDriverClient interface, noAWS dependency.
@temporalio/external-storage-s3-aws-sdk: AwsSdkS3StorageDriverClient, backed by@aws-sdk/client-s3 (peer dep).
@experimentalnot yet wired into the converter pipeline.Why?
client.
Checklist